Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Harmonizing feed reader type #7490

Merged
merged 9 commits into from
Oct 30, 2023
Merged

Harmonizing feed reader type #7490

merged 9 commits into from
Oct 30, 2023

Conversation

Simbiat
Copy link
Contributor

@Simbiat Simbiat commented Oct 21, 2023

Description:

Parser name for feed readers is feed reader, but we had apps with type Feed Reader or even Feed Reader App, which was inconsistent compared to other client types both in terms of case and name. This little change harmonizes things.

Review

Parser name is `feed reader`, but we had some apps with type `Feed Reader` or even `Feed Reader App`, which was inconsistent compared to other client types
@Simbiat
Copy link
Contributor Author

Simbiat commented Oct 29, 2023

Can the checks be rerun? It does not look like they failed because of my change, after all.

@liviuconcioiu
Copy link
Collaborator

Can the checks be rerun? It does not look like they failed because of my change, after all.

If you use Github Desktop, it can be done from the app.

Untitled1

@Simbiat
Copy link
Contributor Author

Simbiat commented Oct 29, 2023

I committed through GitHub website. Cloned the repo in GitHub Desktop, triggered recheck there, and it shows as if the check were scheduled, but I do not see any updates on website.

@liviuconcioiu
Copy link
Collaborator

You can make a test commit and remove it in another commit then push both. It will trigger the checks.

@Simbiat
Copy link
Contributor Author

Simbiat commented Oct 29, 2023

Done. Probably need to report a bug to GitHub, too.

@Simbiat Simbiat mentioned this pull request Oct 30, 2023
@sgiehl
Copy link
Member

sgiehl commented Oct 30, 2023

Doesn't this change make the type flag for feed readers fully obsolete? It would be the same everywhere. So might be better to remove it from the yml file (and maybe set it fixed in PHP if needed)

@Simbiat
Copy link
Contributor Author

Simbiat commented Oct 30, 2023

Yup, no need to keep them. By default we are using $parserName value for this from the looks of it, so only need to modify YAML file.

@Simbiat
Copy link
Contributor Author

Simbiat commented Oct 30, 2023

Interesting, removing the type from YAML fails the tests, though 🤔

No longer required, since it's now fixed value
@Simbiat
Copy link
Contributor Author

Simbiat commented Oct 30, 2023

Test was failing on YAML structure check, not the parser check. Updated the test accordingly.

@sanchezzzhak sanchezzzhak merged commit 0dcf0f7 into matomo-org:master Oct 30, 2023
15 checks passed
@Simbiat Simbiat mentioned this pull request Oct 30, 2023
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants